fix(agent-server): detect all secret-bearing fields for the plaintext-save warning, not just llm.api_key - #4618

🟡 Acceptable — clean design, correct implementation, a couple of symmetric test gaps worth filling.

The config field → helper → store-constructor wiring is consistent and readable. All write paths (save, set_secret, delete_secret via _save_with_versions) properly funnel through the require_cipher guard. The update() path on FileSettingsStore inherits the check automatically because it delegates to save(). The flag is off by default so existing deployments are unaffected. Error messages are actionable.

Two symmetric tests are missing (details in inline comments), and one docstring is pure noise. Otherwise good.

[TESTING GAPS]

• tests/agent_server/test_persistence_require_cipher.py — two symmetric FileSecretsStore coverage holes (see inline).

[STYLE NOTE]

• openhands-agent-server/openhands/agent_server/persistence/store.py, line 854 — redundant docstring (see inline).

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  Default is False; no behaviour change for any existing deployment. When enabled, the guard raises immediately with a clear, actionable message before writing anything. All secret write paths are covered by the same _save_with_versions funnel.

VERDICT:
✅ Worth merging — functionally correct and safe. The two missing tests are worth adding before or just after merge but are not blockers.

KEY INSIGHT:
The security contract (raise before writing, never silently store plaintext when the flag is on) is correctly enforced on every write path; the only gap is symmetric backward-compat test coverage for FileSecretsStore.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.
2. Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

🟡 Acceptable — Clean, minimal, well-tested opt-in guard. One improvement opportunity worth a follow-up.

Summary: This PR adds require_secret_key (OH_REQUIRE_SECRET_KEY) as an opt-in boolean flag that makes FileSettingsStore / FileSecretsStore raise MissingCipherError instead of silently falling back to plaintext when OH_SECRET_KEY is absent. The design is correct — default-off preserves the zero-config OSS path, and the guard is wired through both stores symmetrically. 8 tests exercise the real code paths across all combinations.

One gap worth a follow-up: The settings guard checks llm_api_key_is_set (LLM api_key field only). OpenHandsAgentSettings also has critic_api_key: str | SecretStr | None with its own serialize_secret serializer. A user with only critic_api_key set and require_cipher=True will silently get plaintext output — the guard won't fire. See inline comment for details. This matches the existing warning gate, so it's a pre-existing design boundary, but widening it here would make the guarantee complete.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  Pure opt-in feature with default=False. Default-off means zero behavior change for existing deployments. The only code that executes on the new path is an early raise inside an already-existing if not cipher: branch — no new state, no new I/O. Test coverage is solid and the backward-compat test is explicitly preserved.

VERDICT:
✅ Worth merging — core logic is sound, the critic_api_key gap is a narrow pre-existing edge case that can be addressed in a follow-up once the enterprise opt-in wiring lands.

KEY INSIGHT: The secret-presence gate (llm_api_key_is_set / custom_secrets) is inherited from the existing warning path — broadening it to cover all SecretStr fields would make require_cipher airtight.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

🔴 Taste Rating: Needs improvement — the direction is sound, but the secret-detection predicate still has security-sensitive blind spots.

[CRITICAL ISSUES]

• [openhands-agent-server/openhands/agent_server/persistence/models.py, Line 184] Security/Correctness: has_any_secret only checks the LLM API key and critic API key. Other settings fields serialize secrets too (agent_context.secrets, MCP env/headers, header auth credentials, OAuth token state), so require_cipher=True can still fall through to plaintext persistence for those settings.

[TESTING GAPS]

• Add regression coverage for FileSettingsStore(..., require_cipher=True) with no cipher when agent_context.secrets is populated, and for at least one MCP secret-bearing field. Those tests should fail today because the guard returns False and the store writes plaintext.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM
  This PR touches security-sensitive secret persistence. The default behavior remains unchanged, but the new opt-in guard can give multi-tenant deployments a false guarantee while still allowing some settings secrets to be persisted in plaintext.

VERDICT:
❌ Needs rework: the require_secret_key guard needs to cover every settings secret serializer, not just LLM/critic keys.

KEY INSIGHT:
The store should reuse the existing serialization pipeline to detect secret-bearing settings instead of hand-maintaining a partial list of secret fields.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

🟢 Good taste — Elegant, focused solution that fixes a real gap in secret detection coverage.

What this does

Previously, the plaintext-storage guard only checked llm_api_key_is_set (settings) and custom_secrets non-empty (secrets), meaning MCP server env/headers, critic_api_key, agent_context.secrets, and any future secret-bearing fields were silently persisted in plaintext with no warning. This PR:

1. Broadens detection via has_any_secret, which probes the real Pydantic serialization pipeline instead of field-type inspection — correctly catching AgentContext.secrets whose values are plain str at rest and only become secret-shaped inside their field serializer.
2. Adds require_secret_key config flag (default False) that turns the warning into a hard MissingCipherError raise, intended for multi-tenant / production deployments.
3. Tests all the edge cases: critic key, MCP env, agent context secrets, empty-secrets bypass, cipher round-trips, backward compat.

One minor observation

In the no-cipher path, has_any_secret runs model_dump (probe) before save() runs model_dump again for the actual write — two serializations. Harmless given the filesystem I/O that follows, but worth noting if this path ever becomes latency-sensitive.

The PR description is empty, which makes it harder to understand the motivation from the PR UI, but the code and test comments are self-documenting enough.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  require_secret_key defaults to False, so all existing behavior is fully preserved. The only behavioral difference for existing deployments is that the plaintext warning now fires for more secret types (MCP env vars, critic key, agent context secrets) — a strictly safer default. The MissingCipherError path is entirely opt-in.

VERDICT:
✅ Worth merging — Clean implementation, solid tests, backward-compatible.

KEY INSIGHT:
Using the real serialization pipeline as a probe (a fake cipher that sets a flag on encrypt()) is the right design: it guarantees coverage over all current and future secret-bearing fields without maintaining a fragile field checklist.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.
2. Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

🟢 Taste Rating: Good taste — this is a focused, pragmatic fix that reuses the real secret serialization path instead of maintaining another drift-prone checklist.

Summary

• The require_secret_key/require_cipher path is additive and default-off, so existing zero-config OSS/self-hosted behavior is preserved.
• Secret detection now flows through the same serializers used for persistence, which correctly covers LLM keys, critic keys, MCP secret maps, custom secrets, and agent_context.secrets without adding per-field special cases.
• The new tests exercise both settings and custom-secret stores, plaintext backward compatibility, cipher-backed round-trips, and previously missed non-LLM secret fields. GitHub CI for the PR is green; I could not run the local pytest suite in this sandbox because the checked-out environment lacks pytest/project dependencies, and I avoided installing them during review.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM
  This touches secrets handling and persistence, which is security-sensitive, but the blast radius is constrained: the stricter behavior is opt-in, existing defaults are preserved, no dependencies changed, and the implementation follows the existing Pydantic serializer/cipher pattern with direct regression coverage.

VERDICT:
✅ Worth merging: Core logic is sound, backward-compatible by default, and covered by meaningful regression tests.

KEY INSIGHT:
The probe-cipher approach has good taste because it removes a duplicated notion of “which fields are secrets” and lets the existing serialization data structure own that decision.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

🟢 Taste Rating: Good taste — focused fix that broadens plaintext-save warnings by reusing the existing secret serialization pipeline.

Summary

• has_any_secret uses the real Pydantic secret serializers with a probe cipher, avoiding another drift-prone per-field checklist and catching future secret-bearing fields automatically.
• The store changes are minimal and backward-compatible: plaintext fallback behavior is unchanged; only the warning gate is corrected.
• Regression coverage hits LLM, critic, MCP, agent-context, custom-secret, no-secret, warning, and cipher-backed round-trip paths. I also ran uv run pytest tests/agent_server/test_persistence_secret_detection.py -q locally: 11 passed.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM
  Touches security-sensitive secret persistence/warning logic, but the blast radius is narrow and behavior remains backward-compatible because it only broadens warnings when saving without a cipher.

VERDICT:
✅ Worth merging: Core logic is sound and tested; no blocking code-review issues found.

KEY INSIGHT:
The probe-cipher approach has the right data flow: the same serializers that would persist secrets are now the source of truth for detecting them.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation